Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements a new client API to dynamically switch between hardware and software cryptographic implementations at runtime. The feature allows clients to control whether the server uses hardware acceleration (via crypto callbacks) or pure software wolfCrypt implementations by modifying the server's devId field.
Changes:
- New client API with both blocking and async variants for setting crypto affinity
- Server handler to process affinity change requests with proper validation and error handling
- New message types and translation functions for client-server communication
- Comprehensive test coverage including edge cases and error conditions
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| wolfhsm/wh_server.h | Added configDevId field to preserve original device ID configuration |
| wolfhsm/wh_message_comm.h | Defined message action, enum values, and request/response structures |
| wolfhsm/wh_error.h | Added WH_ERROR_BADCONFIG error code for configuration failures |
| wolfhsm/wh_client.h | Declared new client API functions with comprehensive documentation |
| src/wh_server.c | Implemented initialization of configDevId and request handler with validation |
| src/wh_message_comm.c | Implemented message translation functions following existing patterns |
| src/wh_client.c | Implemented client APIs with proper error handling and retry logic |
| test/wh_test_clientserver.c | Added comprehensive test coverage for various scenarios |
| docs/draft/crypto_affinity.md | Created API documentation with usage examples and return code reference |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
e3fe789 to
571a0c7
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
571a0c7 to
3655e5c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
9949065 to
506f420
Compare
f1de629 to
55fe3ab
Compare
bigbrett
left a comment
There was a problem hiding this comment.
Nice work. Some tweaks required and some deeper architectural questions.
d426556 to
b109054
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 30 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (1)
wolfhsm/wh_server.h:72
- Removing
devIdfromwhServerCryptoContextis an API/ABI breaking change for downstream users that initialize this public struct (previously.devId = INVALID_DEVID). If this is intentional, it should be called out in release notes/migration docs; otherwise consider providing a compatibility shim (e.g., keep the field behind a deprecated macro) to avoid breaking existing integrations.
#ifndef WOLFHSM_CFG_NO_CRYPTO
typedef struct whServerCryptoContext {
#ifndef WC_NO_RNG
WC_RNG rng[1];
#endif
} whServerCryptoContext;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
c72c40c to
cbc3ea7
Compare
14fa4e5 to
4de6b72
Compare
bigbrett
left a comment
There was a problem hiding this comment.
@AlexLanzano looks awesome. Pls remove unnecessary include and run clang-format, then we are good.
wolfhsm/wh_client.h
Outdated
|
|
||
| /* Component includes */ | ||
| #include "wolfhsm/wh_comm.h" | ||
| #include "wolfhsm/wh_message_comm.h" |
wolfhsm/wh_server.h
Outdated
| whCommServer comm[1]; | ||
| #ifndef WOLFHSM_CFG_NO_CRYPTO | ||
| whServerCryptoContext* crypto; | ||
| int defaultDevId; |
There was a problem hiding this comment.
run git-clang-format main and commit changes pls
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ld to generic crypto header
…ity test to new aes cbc functions
4de6b72 to
78735dd
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 32 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
wolfhsm/wh_server.h:1
- The field is named
defaultDevIdwhile the existing codebase consistently usesdevId(e.g.,whServerConfig.devId,crypto->devId). A more consistent name following the existing convention would beconfigDevIdas mentioned in the PR description, or simply match the pattern used elsewhere.defaultDevIdimplies a fallback semantic that is not always accurate (e.g., it is also used directly for SHE and keystore operations regardless of affinity).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 32 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Add SetCryptoAffinity API for runtime HW/SW crypto selection
Summary
This PR adds a new client API to dynamically switch between hardware and software cryptographic implementations at runtime. This allows clients to control whether the server uses crypto callbacks (hardware acceleration) or pure software wolfCrypt implementations.
Changes
New Client API (
wh_client.c,wh_client.h)wh_Client_SetCryptoAffinity()- blocking callwh_Client_SetCryptoAffinityRequest()/wh_Client_SetCryptoAffinityResponse()- async APINew Message Types (
wh_message_comm.c,wh_message_comm.h)WH_MESSAGE_COMM_ACTION_SET_CRYPTO_AFFINITYactionWH_CRYPTO_AFFINITY_SW/WH_CRYPTO_AFFINITY_HWenum valuesServer Handler (
wh_server.c,wh_server.h)configDevIdfield to preserve the original configured device IDdevIdbetweenINVALID_DEVID(SW) andconfigDevId(HW)Tests (
wh_test_clientserver.c)_testCryptoAffinity()to verify SW/HW switching behaviorDocumentation (
docs/draft/crypto_affinity.md)Usage